Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature gate metrics usage in program-runtime #3477

Merged
merged 3 commits into from
Jan 4, 2025
Merged

feature gate metrics usage in program-runtime #3477

merged 3 commits into from
Jan 4, 2025
+34 −15

Conversation

kevinheavey
Copy link

@kevinheavey kevinheavey commented Nov 5, 2024 •
edited
Loading

Problem

solana-program-runtime has an annoying solana-metrics dependency that severely impacts build time and dependency hell. It's particularly annoying for litesvm which depends on solana-program-runtime

Summary of Changes

Put solana-metrics behind a feature gate in solana-program-runtime. Activate the metrics feature for crates that should use it

Closes #3390

@kevinheavey kevinheavey mentioned this pull request Nov 5, 2024
Closed
@buffalojoec
Copy link

@kevinheavey you mentioned doing this doesn't really help your project without breaking up the SVM crate.
#3390 (comment)

Is there another direct reason this change should go into program-runtime?

@kevinheavey
Copy link
Author

Yeah I realised can just copy paste what I need from the svm crate, so this would still be an improvement

@kevinheavey kevinheavey force-pushed the program-runtime-metrics-feature-gate branch from c892653 to 4ec02d1 Compare December 7, 2024 00:34
@kevinheavey kevinheavey requested a review from a team as a code owner December 7, 2024 00:34
@kevinheavey
Copy link
Author

Build times on my machine:

Metrics feature enabled: 31 seconds
Metrics feature disabled: 16 seconds

@kevinheavey kevinheavey force-pushed the program-runtime-metrics-feature-gate branch from 4ec02d1 to fcf7809 Compare December 12, 2024 10:22
@kevinheavey kevinheavey force-pushed the program-runtime-metrics-feature-gate branch from fcf7809 to e9ff84e Compare December 19, 2024 16:58
@kevinheavey kevinheavey mentioned this pull request Dec 31, 2024
Open
@kevinheavey kevinheavey force-pushed the program-runtime-metrics-feature-gate branch 2 times, most recently from c453d11 to 3246d0e Compare January 1, 2025 14:31
@kevinheavey
Copy link
Author

@buffalojoec any blockers here?

buffalojoec
buffalojoec reviewed Jan 3, 2025
View reviewed changes
Copy link

@buffalojoec buffalojoec left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me. I think it's a great way to get the metrics dependency out for those that need to build without it.

The only part I'm a little iffy on is the feature-flagged parameter. I only found one other spot in the codebase where we do this.

agave/runtime/src/bank.rs

Lines 2711 to 2712 in 4d87254

#[cfg(feature = "dev-context-only-utils")] collector_id_for_tests: Option<Pubkey>,
#[cfg(feature = "dev-context-only-utils")] genesis_hash: Option<Hash>,

@Lichtso are you opposed to this convention in the program runtime?

Also, as @LucasSte pointed out on another one of my PRs, I think we also need to add this new feature to CI somewhere to make sure it's covered in the tests. Right now, program-runtime doesn't have a unit test for ProgramCacheEntry::new or ProgramCacheEntry::reload, but it likely will someday, and we want to avoid any footguns.

@LucasSte
Copy link

LucasSte commented Jan 3, 2025 •
edited
Loading

Also, as @LucasSte pointed out on another one of my PRs, I think we also need to add this new feature to CI somewhere to make sure it's covered in the tests. Right now, program-runtime doesn't have a unit test for ProgramCacheEntry::new or ProgramCacheEntry::reload, but it likely will someday, and we want to avoid any footguns.

We should add it to dummy-for-ci-check, so that cargo-clippy can detect any potential problems during CI.

@kevinheavey
Copy link
Author

We should add it to dummy-for-ci-check

Do you mean we should add a dummy-for-ci-check feature to program-runtime?

@kevinheavey kevinheavey force-pushed the program-runtime-metrics-feature-gate branch from 3246d0e to 4159fae Compare January 3, 2025 18:41
@LucasSte
Copy link

LucasSte commented Jan 3, 2025

We should add it to dummy-for-ci-check

Do you mean we should add a dummy-for-ci-check feature to program-runtime?

Yes, then you include the metrics feature in the dummy feature so that CI properly checks that everything is correct.

@kevinheavey kevinheavey force-pushed the program-runtime-metrics-feature-gate branch from ede141c to fb745cc Compare January 3, 2025 21:52
@kevinheavey kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Jan 4, 2025
@mergify mergify bot merged commit 10be2c3 into anza-xyz:master Jan 4, 2025
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buffalojoec buffalojoec buffalojoec approved these changes

Assignees
No one assigned
Labels
automerge automerge Merge this Pull Request automatically once CI passes need:merge-assist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevinheavey @buffalojoec @LucasSte